Skip to content

Conversation

aaronjeline
Copy link
Collaborator

Main question I have is on my const_cast at checkedregions.c:173. Is there a way to avoid this, without needing to change the entire project to make ProgramInfo::getVariable const?

if (isTopLevel(Context, S)) {
const auto &Parents = Context->getParents(*S);
assert(!Parents.empty());
FunctionDecl* Parent = const_cast<FunctionDecl*>(Parents[0].get<FunctionDecl>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does Parent need to be non-const ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because ProgramInfo::getVariable takes a non-const

Copy link
Collaborator

@john-h-kastner john-h-kastner Dec 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might be able to make the parameter of getVariable const. We should rarely if ever be mutating this sort of clang AST node object. Perhaps not as part of this PR, but in a later change we could add const annotations to these AST node objects where we can.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm happy to go through and try to make that change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, per the status meeting: Let's just do this as a separate PR; leave the const_cast for now.

@aaronjeline aaronjeline changed the title CheckedRegions understnad return types, fixes #263 CheckedRegions understand return types, fixes #263 Dec 18, 2020
Copy link
Member

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with this. Per the comment on the PR, change getVariable's type as part of another PR.

@aaronjeline aaronjeline merged commit 6531b2e into master-post-microsoft Dec 18, 2020
@Machiry Machiry deleted the cr branch January 22, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants